fix: allow omitted task push config id - #1092
Conversation
kabir
left a comment
kabir
left a comment
There was a problem hiding this comment.
Hi @hutiefang76 - thank you for the PR 👍
I found a few small issues in the code, but we also need to take JpaDatabasePushNotificationConfigStore into account and make some changes, and test this inline with what you have done here. JpaPushNotificationConfigStoreTest might be the right place.
Also GetTaskPushNotificationConfigParams asserts id non-null despite its Javadoc calling it optional. Might as well fix this since it is related.
| public void testSetInfoWithoutConfigId() { | ||
| String taskId = "task1"; | ||
| TaskPushNotificationConfig initialConfig = TaskPushNotificationConfig.builder() | ||
| .id("") // No ID set |
There was a problem hiding this comment.
Please create a new test rather than changing this existing one.
Removing .id("") repurposes this test from the empty-string path to the null path rather than covering both.
The empty-string case is the one production actually hits — the mapper doesn't apply emptyToNull to id, so at runtime a missing id arrives as "", never null.
Recommend keeping the "" case here and adding a separate null-id test (or parameterizing over both), so the load-bearing branch stays asserted.
There was a problem hiding this comment.
The previous comment should still be addressed.
| assertEquals(taskId, configResult.configs().get(0).id()); | ||
|
|
||
| TaskPushNotificationConfig updatedConfig = TaskPushNotificationConfig.builder() | ||
| .id("") // No ID set |
There was a problem hiding this comment.
Please create a new test rather than changing this existing one.
There was a problem hiding this comment.
The previous comment should still be addressed.
| public TaskPushNotificationConfig setInfo(TaskPushNotificationConfig notificationConfig) { | ||
| String taskId = Assert.checkNotNullParam("taskId", notificationConfig.taskId()); | ||
| TaskPushNotificationConfig.Builder builder = TaskPushNotificationConfig.builder(notificationConfig); | ||
| if (notificationConfig.id().isEmpty()) { |
There was a problem hiding this comment.
Not a comment about this code, but JpaDatabasePushNotificationConfigStore has the same latent NPE, and needs fixing too.
|
Updated in 5bd4916.
Verified on JDK 17 with: Result: 2 spec tests, 23 request-handler tests, and 16 JPA-store tests passed (one pre-existing disabled test).` |
| public void testSetInfoWithoutConfigId() { | ||
| String taskId = "task1"; | ||
| TaskPushNotificationConfig initialConfig = TaskPushNotificationConfig.builder() | ||
| .id("") // No ID set |
There was a problem hiding this comment.
The previous comment should still be addressed.
| assertEquals(taskId, configResult.configs().get(0).id()); | ||
|
|
||
| TaskPushNotificationConfig updatedConfig = TaskPushNotificationConfig.builder() | ||
| .id("") // No ID set |
There was a problem hiding this comment.
The previous comment should still be addressed.
|
Addressed in dab6633.
Validated with |
|
Sorry @hutiefang76 we're doing a lot of merging today, and there is now a conflict :-) |
dab6633 to
e4a3d37
Compare
|
Rebased onto current |
2 participants
Description
Allow
TaskPushNotificationConfigto omit itsidwhen a client creates a push notification configuration, matching the A2A specification. The in-memory store now assigns the task ID when the supplied ID is null or empty, preserving its existing defaulting behavior.Tests
mvn -pl spec,server-common -am -Dtest=TaskPushNotificationConfigTest,InMemoryPushNotificationConfigStoreTest -Dsurefire.failIfNoSpecifiedTests=false testTaskPushNotificationConfigTest: 1 passedInMemoryPushNotificationConfigStoreTest: 33 passedFollow the CONTRIBUTING guide
Use a conventional commit title
Tests pass
README changes are not needed for this API-contract correction
Fixes #1081 🦕